-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Try/typer reorg #47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Try/typer reorg #47
Conversation
There's a typo in the second-to-last commit title. |
Also the description of the second-to-last commit has weird line breaks when rendered in Github. Seems that it's caused by the first line of the description being 110 characters long. Typical line length expected by Git-related tools is 80 characters. |
There's also a typo in the second commit description. |
Btw it's really cool to see these really fine-grained changes with all the explanations. Do all the tests pass for the individual commits? |
+1 on enjoying the details in the commit messages our guidelines for commit titles are: max 60 chars for the first line, should be written using active verbs, so that it makes sense as a bullet item in release notes (i.e., it's the answer to the question how this commit improves the codebase) |
Thanks for the tips on commit formatting. Is there a way to do this in the existing commits (preferably on github itself), or do I have to create new ones? |
There's no way on GitHub directly, AFAIK. With git you can of course simply do |
OK, I think I'll take in the advice for future commits, but won't change the present ones. |
Rewrap needs to produce alternatives with signatures. Otgerwise the new denotation will simply overwrite the old because both the overloaded TermRef and the alternative will hash to the same unique TermRef.
Overloaded TermRefs do not have an info, and consequently do not support =:=. Yet in Typer#checkNewOrShadowed we compared termrefs with =:=. This gives an exception if the termrefs are overloaded. The fix is to provide a new method isSameRef in TypeComparer which is called instead of =:= in Typer#checkNewOrShadowed.
Goal is better modularization and avoiding code duplication and divergence between Typer and tpd. As a first step, we split Inferencing into Inferencing, Checking, and ProtoTypes. Inferencing and Checking become Typer traits, while ProtoTypes remains a global object. Eventually: - we want to have a SimpleTyper, which does the main stuff in tpd, and can mixin either full checking or no checking. Each method in SimpleTyper takes an untyped tree (which is assumed to have typed arguments) and adds a toplevel type to that tree. The methods subsume the type-checking parts in Typers, except for (1) simplifications and expansions (2) computing prototypes and recursing with them into childtrees (3) adaptation. The method calls the necessary checking operations, which may however be stubbed out. The idea is already exercised in the typechecking code for Literal and New, except that for now it calls methods in tpd (they will become methods in SimpleTyper instead). - Typer should inherit from SimpleTyper, and forward all logic except for (1) - (3) to it. - tpd should call the simple typer it gets from ctx.typer - ctx.typer should be a SimpleTyper, not a complete one.
TypeAssigners assign a toplevel type to a node. They are mixed into Typer, and can be accessed from tpd using ctx.typeAssigner.
Common code between tpd and Typer has been factored out into class TypeAssigner.
Rebased all commits on current master. |
val qual1 = followTypeAlias(qual) | ||
if (qual1.isEmpty) notAnExtractor(sel) | ||
else trySelectUnapply(qual1)(_ => notAnExtractor(sel)) | ||
} | ||
|
||
def fromScala2x = unapply.symbol.exists && (unapply.symbol.owner is Scala2x) | ||
def fromScala2x = unapplyFn.symbol.exists && (unapplyFn.symbol.owner is Scala2x) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"fromScala2x" feels like a conversion, maybe "isFromScala2x"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
LGTM |
Implement flow typing using nullability check; Update documents
A rather sweeping commit of typer structure. Main goal: Avoid code duplication and possible divergence of logic between tpd and Typer.
I needed the previous fix/overloadedAccess pull request to make the code in this one compile under dotty. I removed that former pull request and combined the two, so that we can get this in quickly.
Review by @DarkDimius @samuelgruetter